Skip to content

Rework required components #20110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Rework required components #20110

wants to merge 16 commits into from
+1,598 −1,342

Conversation

SkiFire13
Copy link
Contributor

@SkiFire13 SkiFire13 commented Jul 13, 2025 •
edited
Loading

Objective

Solution

  • Switch everything to a depth-first order priority as @cart originally intended it.
  • Effectively revert Use register_dynamic for merging #18028, which tried to give priority to components with higher inheritance depth (i.e. with a breadth-first order priority).

Testing

Most existing tests pass, except a couple that were removed due to testing inheritance depth or the breadth-first order priority that this PR will remove.

Some tests were also added, some of which as regression tests and others to add more coverage.

@SkiFire13 SkiFire13 force-pushed the rework-required-components branch 2 times, most recently from 5362a26 to 8b14b9c Compare July 13, 2025 14:48
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events S-Blocked This cannot move forward until something else changes C-Bug An unexpected or incorrect behavior X-Contentious There are nontrivial implications that should be thought through labels Jul 14, 2025
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jul 14, 2025
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Jul 14, 2025
@SkiFire13 SkiFire13 force-pushed the rework-required-components branch from 8b14b9c to 18c4ca4 Compare July 14, 2025 18:39
@alice-i-cecile
Copy link
Member

@SkiFire13 is this ready for review now that the bundle-split PR is merged? This is still in draft mode :)

@alice-i-cecile alice-i-cecile requested review from cart and Jondolf July 14, 2025 19:33
@SkiFire13 SkiFire13 marked this pull request as ready for review July 14, 2025 19:48
@SkiFire13
Copy link
Contributor Author

SkiFire13 commented Jul 14, 2025 •
edited
Loading

@SkiFire13 is this ready for review now that the bundle-split PR is merged? This is still in draft mode :)

Ah yes, I was forgetting to change the status! I think it might still need some more tests, but the implementation should be ready.

@urben1680 urben1680 mentioned this pull request Jul 15, 2025
Open
@urben1680
Copy link
Contributor

I encountered an issue where a required component is not registered at all on main during my work on #19852. This PR seems to fix that too. That problem seems to be not among the issues listed here.

I try to review this the next days and if needed suggest another test if this is missing.

@SkiFire13 SkiFire13 mentioned this pull request Jul 17, 2025
Open
@SkiFire13
Copy link
Contributor Author

I added a few more tests, cleaned up some parts of the code and added a fix for #20173 (I would have preferred a separate PR but that ended up being requiring too many changes that would be included in this PR anyway)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElliottjPierce ElliottjPierce Awaiting requested review from ElliottjPierce

@cart cart Awaiting requested review from cart

@Jondolf Jondolf Awaiting requested review from Jondolf

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
0.17
3 participants
@SkiFire13 @alice-i-cecile @urben1680